Editorial: Refactor time zone identifier spec text by justingrant · Pull Request #2573 · tc39/proposal-temporal

justingrant · 2023-05-04T07:42:38Z

This PR refactors how the Temporal spec deals with time zone identifiers. Summary is below.

This PR is a companion to the just-merged tc39/ecma262#3035 which performed a similar refactor in ECMA-262.

Changes to 262 section of the Temporal spec

Minor editorial edits to ECMA-262's new Time Zone Identifiers section and AvailableNamedTimeZoneIdentifiers AO.
Adopts the now-official rename of DefaultTimeZone to SystemTimeZoneIdentifier (It was renamed in 262 to more clearly match what it does. Also because Temporal doesn't have a "default" time zone like Date has.)
Removes AvailableTimeZoneNames, CanonicalizeTimeZoneName and IsAvailableTimeZoneName, and replaces them with calls to a new AO GetAvailableTimeZoneIdentifier.

Changes to 402 section of the Temporal spec

TZDB-related spec text, specifically dealing with Zone and Link identifiers, TZDB updating, TZDB build options, etc. Much of this text is non-normative recommendations (to encourage implementers to avoid TZDB footguns) which can later be tightened into normative requirements as part of proposal-canonical-tz.
The 402 version of AvailableTimeZoneIdentifiers.
Removes AvailableTimeZoneIdentifiers, CanonicalizeTimeZoneName and IsAvailableTimeZoneName, and replaces them with calls to GetAvailableTimeZoneIdentifier. A later editorial PR to 402 will make the same changes there, and also replace its new AvailableCanonicalTimeZoneNames AO with an AvailableTimeZoneIdentifiers-based version.

When reviewing this PR, remember that many of the changes are removing spec text, which in the 402 section of the spec causes a confusing diff because the text isn't actually removed from the file but rather is just put into a larger <del> block. I added review notes in the spec text to make this clearer.

This PR also includes polyfill changes to more closely match the new spec text.

codecov · 2023-05-04T07:50:29Z

Codecov Report

Merging #2573 (192acba) into main (d2b6d50) will decrease coverage by 0.02%.
The diff coverage is 95.20%.

@@            Coverage Diff             @@
##             main    #2573      +/-   ##
==========================================
- Coverage   96.15%   96.13%   -0.02%     
==========================================
  Files          20       20              
  Lines       11480    11580     +100     
  Branches     2170     2205      +35     
==========================================
+ Hits        11039    11133      +94     
- Misses        377      383       +6     
  Partials       64       64

Impacted Files	Coverage Δ
polyfill/lib/intl.mjs	`97.84% <83.33%> (-1.03%)`	⬇️
polyfill/lib/ecmascript.mjs	`98.26% <98.82%> (+<0.01%)`	⬆️
polyfill/lib/timezone.mjs	`100.00% <100.00%> (ø)`
polyfill/lib/zoneddatetime.mjs	`100.00% <100.00%> (ø)`

ptomato

In general, looks pretty good to me. I have some minor comments about phrasing. Looking forward to seeing what happens with this after the direction of the ECMA-262 PR becomes clearer.

justingrant

Thanks for the review! Other than the recommended/required comments, I agree with all your feedback and will make those changes. Let's discuss the recommendation thing further.

Looking forward to seeing what happens with this after the direction of the ECMA-262 PR becomes clearer.

Based on my last convo with the 262 editors, the 262 PR is likely to be a subset of what's here. Planning to update the 262 PR tomorrow with a new draft of that subset.

But I don't anticipate major changes to the content in this PR... main questions seem to be about where and when the text will go (262 now vs. 262 and 402 later via Temporal) not necessarily about the text itself.

This commit simplifies and clarifies spec text related to time zone identifiers. Goals include making it easier to integrate Temporal into ECMA-262 soon, and simplifying both Temporal and ECMA-402 spec text by centralizing common logic in ECMA-262. If this commit is merged into ECMA-262, editorial PRs will follow for Temporal (draft: tc39/proposal-temporal#2573) and ECMA-402 (draft TBD) to leverage the updated spec text and AOs to simplify those specs. Changes: * Adds editorial spec text explaining how time zone identifiers work in ECMAScript, and pointing readers to 402 for implementations that use the IANA TimeZoneDatabase. * Adds definitions related to time zone identifiers. * Renames `DefaultTimeZone` to `SystemTimeZoneIdentifier` in order to more clearly match its intent. * Removes the need to override `SystemTimeZoneIdentifier` in ECMA-402. * Renames variables named _timeZone_ to _timeZoneIdentifier_ to avoid future ambiguity with `Temporal.TimeZone`. * Adds text that more clearly explains existing spec text allowing non-402 implementations to support non-UTC time zones. * Adds new abstract operation `AvailableTimeZoneIdentifiers`which, along with the existing (renamed to) `SystemTimeZoneIdentifier`, enables all ECMA-402 and Temporal operations related to time zone identifiers to be implemented on top of these AOs.

This commit simplifies and clarifies spec text related to time zone identifiers. Goals include making it easier to integrate Temporal into ECMA-262 soon, and simplifying both Temporal and ECMA-402 spec text by centralizing common logic in ECMA-262. If this commit is merged into ECMA-262, editorial PRs will follow for Temporal (draft: tc39/proposal-temporal#2573) and ECMA-402 (draft TBD) to leverage the updated spec text and AOs to simplify those specs. Changes: * Adds editorial spec text explaining how time zone identifiers work in ECMAScript, and pointing readers to 402 for implementations that use the IANA TimeZoneDatabase. * Adds definitions related to time zone identifiers. * Renames `DefaultTimeZone` to `SystemTimeZoneIdentifier` in order to more clearly match its intent. * Removes the need to override `SystemTimeZoneIdentifier` in ECMA-402. * Renames variables named _timeZone_ to _timeZoneIdentifier_ to avoid future ambiguity with `Temporal.TimeZone`. * Adds text that more clearly explains existing spec text allowing non-402 implementations to support non-UTC time zones. * Adds new abstract operation `AvailableNamedTimeZoneIdentifiers` which, along with the existing (renamed) `SystemTimeZoneIdentifier`, enables all ECMA-402 and Temporal operations related to time zone identifiers to be implemented on top of these AOs.

justingrant · 2023-06-08T06:14:14Z

+  </emu-clause>
+  </ins>
+
+  <del class="block">


Note that this entire section (the following ~70 lines) is deleted from ECMA-402 and replaced with the new section above (and with new AOs in 262 like AvailableTimeZoneIdentifiers and GetAvailableTimeZoneIdentifier).

So don't bother reviewing anything about the text in here except validating that it's OK to remove these AOs.

justingrant · 2023-06-08T06:23:24Z

+        <p>
+          Time zones in ECMAScript are represented by <dfn variants="time zone identifier">time zone identifiers</dfn>, which are Strings composed entirely of code units in the inclusive interval from 0x0000 to 0x007F.
+          Time zones supported by an ECMAScript implementation may be <dfn variants="available named time zone">available named time zones</dfn>, represented by the [[Identifier]] field of the Time Zone Identifier Records returned by AvailableNamedTimeZoneIdentifiers, or <dfn variants="offset time zone">offset time zones</dfn>, represented by Strings for which IsTimeZoneOffsetString returns *true*.
+          <ins>Time zone identifiers are compared using ASCII-case-insensitive comparisons.</ins>


The 262 editors wanted us to move this line into Temporal because it wasn't needed to support current 262 text.

justingrant · 2023-06-08T06:27:11Z

-      </emu-alg>
-    </emu-clause>
-  </emu-clause>
+  <emu-clause id="sec-date-objects">


This section has a lot of green text, but most of it duplicates what's newly added to ECMA-262, so you don't have to review or even read it all if you don't want to.

To make it easier to spot the changes, I added notes below for the three small places where the text is changed from current 262.

justingrant · 2023-06-11T10:49:55Z

Thanks @gibson042 for the thorough review! I made most of the changes you recommended, suggested that some of them were out of scope, and had a few questions and follow-ups. @ptomato I'll look for your review next week. Thanks!

justingrant · 2023-06-11T22:54:25Z

-        <p>This definition supersedes the definition provided in <emu-xref href="#sec-canonicalizetimezonename"></emu-xref>.</p>
-      </ins>
+    <emu-clause id="sup-defaulttimezone" oldids="sec-defaulttimezone" type="implementation-defined abstract operation">
+      <h1>DefaultTimeZone ( ): a String</h1>


The only reason this shows up in green is because it's being deleted from 402. Can ignore.

ptomato

I wonder if we can just not have the CLDR timezone database? It seems acceptable to be less exhaustive there.

Other than that, only a few Ecmarkup syntax nitpicks. I didn't review the prose as carefully as last time, I figure it's already been scrutinized plenty by the ECMA-262 editors.

This commit builds on the new ECMA-262 text for time zone identifiers that was introduced in tc39/ecma262#3035. This commit adds Temporal-specific AOs and makes a handful of edits to ECMA-262 AOs for changes that didn't apply to %Date% but that will be required after Temporal is merged into ECMA-262. This commit also revises the ASCII-case-insensitive section to address feedback from ECMA-262 editors before this text was removed from tc39/ecma262#3035: * Remove the "sequence of code points" info because only Strings seemed to use these definitions. * Minor wordsmithing, e.g. "string value" => "String"

justingrant · 2023-06-12T22:20:06Z

PR is rebased on main and all review comments are resolved, except for @ptomato's concern about testing the entire list of CLDR IDs. But I think we should keep those in for now, and then consider removing them if proposal-canonical-tz moves forward far enough for Test262 to be able to validate case normalization. So unless there are objections, I'd like to merge this PR as-is (so I can unblock other stuff stacked on this PR) and revisit later if needed.

Holler if this is a problem, otherwise I'll plan to merge later today. Thanks!

justingrant requested review from gibson042 and ptomato May 4, 2023 07:42

justingrant force-pushed the tz-identifiers-refactor branch 2 times, most recently from efa9806 to a12431e Compare May 4, 2023 23:41

ljharb reviewed May 5, 2023

View reviewed changes

Comment thread spec/intl.html Outdated

Comment thread spec/intl.html Outdated

Comment thread spec/intl.html Outdated

justingrant force-pushed the tz-identifiers-refactor branch from 01031a1 to 83d02b5 Compare May 6, 2023 04:38

ptomato reviewed May 8, 2023

View reviewed changes

Comment thread spec/intl.html Outdated

Comment thread spec/intl.html Outdated

Comment thread spec/intl.html Outdated

Comment thread spec/timezone.html Outdated

Comment thread spec/timezone.html Outdated

Comment thread spec/timezone.html Outdated

Comment thread spec/intl.html Outdated

justingrant commented May 10, 2023

View reviewed changes

Comment thread spec/intl.html Outdated

Comment thread spec/intl.html Outdated

This was referenced May 10, 2023

Editorial: simplify and clarify time zone identifier spec text tc39/ecma262#3035

Merged

Add lifetime-of-agent note in CanonicalizeTimeZoneName #2493

Closed

justingrant force-pushed the tz-identifiers-refactor branch 2 times, most recently from 4327354 to a131620 Compare May 11, 2023 05:59

justingrant force-pushed the tz-identifiers-refactor branch 3 times, most recently from c94507c to daca88c Compare May 18, 2023 05:35

justingrant force-pushed the tz-identifiers-refactor branch 10 times, most recently from de702fb to 2f3d98c Compare May 23, 2023 02:41

justingrant force-pushed the tz-identifiers-refactor branch from 68a71e3 to aac6066 Compare June 8, 2023 06:11

justingrant commented Jun 8, 2023

View reviewed changes

justingrant force-pushed the tz-identifiers-refactor branch from aac6066 to f02f5be Compare June 8, 2023 06:18

justingrant commented Jun 8, 2023

View reviewed changes

Comment thread spec/mainadditions.html Outdated

justingrant commented Jun 8, 2023

View reviewed changes

Comment thread spec/mainadditions.html Outdated

justingrant commented Jun 8, 2023

View reviewed changes

Comment thread polyfill/runtest262.mjs

gibson042 reviewed Jun 8, 2023

View reviewed changes

justingrant force-pushed the tz-identifiers-refactor branch 2 times, most recently from aad99c8 to d55945b Compare June 11, 2023 10:40

justingrant force-pushed the tz-identifiers-refactor branch from d55945b to c6a8a54 Compare June 11, 2023 22:50

justingrant commented Jun 11, 2023

View reviewed changes

gibson042 mentioned this pull request Jun 12, 2023

Editorial: Introduce SystemUTCEpochMilliseconds to confine GetGlobalObject() #2603

Merged

gibson042 reviewed Jun 12, 2023

View reviewed changes

Comment thread polyfill/test/ecmascript.mjs Outdated

gibson042 reviewed Jun 12, 2023

View reviewed changes

Comment thread polyfill/test/ecmascript.mjs Outdated

justingrant force-pushed the tz-identifiers-refactor branch from c6a8a54 to 948481e Compare June 12, 2023 04:29

ptomato approved these changes Jun 12, 2023

View reviewed changes

Comment thread spec/intl.html Outdated

Comment thread spec/timezone.html Outdated

Comment thread polyfill/test/ecmascript.mjs

justingrant force-pushed the tz-identifiers-refactor branch 4 times, most recently from 46c3b21 to 63492fc Compare June 12, 2023 22:10

justingrant force-pushed the tz-identifiers-refactor branch from 63492fc to 4224ae6 Compare June 12, 2023 22:13

justingrant force-pushed the tz-identifiers-refactor branch from 4224ae6 to dc088aa Compare June 13, 2023 04:06

Polyfill: Refactor time zone identifier handling

192acba

justingrant force-pushed the tz-identifiers-refactor branch from dc088aa to 192acba Compare June 13, 2023 04:28

ptomato merged commit 076f287 into tc39:main Jun 13, 2023

Conversation

justingrant commented May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes to 262 section of the Temporal spec

Changes to 402 section of the Temporal spec

Uh oh!

codecov bot commented May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ptomato left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justingrant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

justingrant Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justingrant Jun 8, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

justingrant Jun 8, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justingrant commented Jun 11, 2023

Uh oh!

justingrant Jun 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ptomato left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justingrant commented Jun 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

justingrant commented May 4, 2023 •

edited

Loading

codecov bot commented May 4, 2023 •

edited

Loading

justingrant Jun 8, 2023 •

edited

Loading